[EVM] Support for spilling constants to memory #885
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #885 +/- ##
=======================================
Coverage 30.03% 30.04%
=======================================
Files 2430 2431 +1
Lines 704105 704196 +91
Branches 176501 176520 +19
=======================================
+ Hits 211480 211571 +91
- Misses 448676 448679 +3
+ Partials 43949 43946 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PavelKopyl
force-pushed
the
kpv-spill-big-constants
branch
2 times, most recently
from
f14e6fc to
293b9ef
Compare
|
📊 Excel Report Available Benchmarks measured for:
|
PavelKopyl
force-pushed
the
kpv-spill-big-constants
branch
from
293b9ef to
fc6d80a
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
PavelKopyl
force-pushed
the
kpv-spill-big-constants
branch
2 times, most recently
from
2dd2ebb to
c4fb743
Compare
PavelKopyl
force-pushed
the
kpv-spill-big-constants
branch
from
c4fb743 to
a5d2fa1
Compare
PavelKopyl
force-pushed
the
kpv-spill-big-constants
branch
from
a5d2fa1 to
91de0ee
Compare
|
📊 evm compiler-tester excel report Benchmarks measured for:
|
|
|
PavelKopyl
force-pushed
the
kpv-spill-big-constants
branch
6 times, most recently
from
5a40615 to
7098b6d
Compare
PavelKopyl
force-pushed
the
kpv-spill-big-constants
branch
2 times, most recently
from
9c8da3c to
3a2fa00
Compare
…pipeline This allows other passes to modify or adjust frame indexes before finalization.
The constant spilling mechanism is divided between two components:
- ConstantUnfolder
Determines which constants are profitable to spill. For these constants,
it emits codegen-only 'IMM_RELOAD Imm' instructions.
- ConstantSpiller
Inserts spills at the beginning of the entry machine function and replaces
IMM_RELOAD instructions with the actual reloads. It also calculates the
required size of the constants’ spill area in the heap. ConstantSpiller
is called from EVMFinalizeStackFrames so that the actual offsets to the
spill area can be computed.
PavelKopyl
force-pushed
the
kpv-spill-big-constants
branch
from
3a2fa00 to
78341c6
Compare
akiramenai
requested changes
Oct 28, 2025
| // spilling after stackification. This helps reduce compilation time, | ||
| // since otherwise we would need to compile roughly twice as many | ||
| // contracts. | ||
| CanSpill = any_of(MFs, |
Collaborator
There was a problem hiding this comment.
Maintainability: Can we use early exit here and emptiness of ImmToUseCount in isSpillEligible?
Contributor
Author
There was a problem hiding this comment.
Yeap, thank you.
| // | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This file identifies IMM_RELOAD instructions representing spilled constants |
Collaborator
There was a problem hiding this comment.
Why IMM_RELOAD if term constant is also use and constant is the correct term?
Contributor
Author
There was a problem hiding this comment.
Changed to CONSTANT_RELOAD.
| MachineBasicBlock::iterator InsertBefore, | ||
| const EVMInstrInfo *TII, const APInt &Imm, | ||
| LLVMContext &Ctx, const DebugLoc &DL) { | ||
| unsigned Opc = EVM::getPUSHOpcode(Imm); |
Collaborator
There was a problem hiding this comment.
Maintainability: Similar logic already exists in EVMConstantUnfolding.cpp and probably other EVM::getPUSHOpcode call sites. Can we unify it?
| LLVMContext &Ctx = EntryMF.getFunction().getContext(); | ||
| const EVMInstrInfo *TII = EntryMF.getSubtarget<EVMSubtarget>().getInstrInfo(); | ||
|
|
||
| DenseMap<APInt, uint64_t> ConstantToSpillOffset; |
Collaborator
There was a problem hiding this comment.
Maintainability: Spill offset == Num of const in ConstantToUseCount map * 32. Do we need a separate map for it?
Contributor
Author
There was a problem hiding this comment.
It's convenient to have a map with final offsets because, as we refer to this info twice. The second time if when we walk over reloads.
| SpillOffset += SpillSlotSize; | ||
| } | ||
|
|
||
| // Emit constant stores in prologue of the entry function. |
Collaborator
There was a problem hiding this comment.
We can probably elaborate this with a call graph and CFG analysis to make the pass more practical. Can we create a follow-up ticket + TODO?
Contributor
Author
There was a problem hiding this comment.
Right, there could be several strategies. For example, we could compute the frequency of constants and perform spills at the machine function (MF) level.
However, in optimized builds (e.g., with -O3), many functions are inlined into the entry function, so inserting the constant-spilling code at the beginning of __entry seems like a reasonable approach.
| << ", at offset: " << Offset << '\n'; | ||
| }); | ||
|
|
||
| BuildMI(SpillMBB, SpillMBB.begin(), DebugLoc(), TII->get(EVM::MSTORE_S)); |
Collaborator
There was a problem hiding this comment.
Maintainability: Reverse order complicates comprehension.
| cl::init(0), | ||
| cl::desc("EVM metadata size")); | ||
|
|
||
| static cl::opt<unsigned> ConstantSpillThreshold( |
Collaborator
There was a problem hiding this comment.
Could you explain why we need to couple spilling and materialization logic? How one affects the other?
Contributor
Author
There was a problem hiding this comment.
I've replaced ConstantSpillThreshold with ConstantReloadThreshold, or you mean why this options is in the ConstantUnfolding file?
Contributor
vladimirradosavljevic
left a comment
vladimirradosavljevic
left a comment
There was a problem hiding this comment.
I can see some performance degradations with this patch, and some nice size improvements, so should we only enable this for size optimizations?
| addPass(createEVMOptimizeLiveIntervals()); | ||
| addPass(createEVMSingleUseExpression()); | ||
| addPass(createEVMBPStackification()); | ||
| addPass(&StackSlotColoringID); |
Contributor
There was a problem hiding this comment.
You don't need to move StackSlotColoringID, as this optimization works only with LiveIntervals that are only added in the EVMBPStackification.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Code Review Checklist
Purpose
Ticket Number
Requirements
Implementation
Logic Errors and Bugs
code does not behave as intended?
that could break the code?
Error Handling and Logging
be added or removed?
written in a way that allows for easy
debugging?
Maintainability
Dependencies
Security
Performance
system performance?
performance of the code significantly?
Testing and Testability
that should be tested in addition?
Readability
smaller methods?
different function, method or variable names?
file/folder/package?
restructured to have a more intuitive control
flow?
better?
understandable?
Documentation
Best Practices
Experts' Opinion
expert or a usability expert, should look over
the code before it can be accepted?